feat(select): add labelPlacement prop, justify prop, and rich content styles for select options#31138
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| height: 40px; | ||
| } | ||
|
|
||
| // Icon |
There was a problem hiding this comment.
Since MD does not have an official select option, I determined the values based on the list component it has.
Icons are considered to be a supported, the size can be found under "Tokens & specs > Token > Size and typography".
| height: 24px; | ||
| } | ||
|
|
||
| // Image / SVG / Thumbnail |
There was a problem hiding this comment.
Since MD does not have an official select option, I determined the values based on the list component it has.
Images are considered to be a supported, the size can be found under "Tokens & specs > Token > Size and typography".
| height: 56px; | ||
| } | ||
|
|
||
| // Video |
There was a problem hiding this comment.
Since MD does not have an official select option, I determined the values based on the list component it has.
Videos are considered to be a supported, the size can be found under "Tokens & specs > Token > Size and typography".
thetaPC
changed the title
thetaPC
changed the title
There was a problem hiding this comment.
Values are based on MD2 specs for dialogs.
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
Great work!! Some subtle changes in here but they make a huge difference. I love it! 🎉
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looking awesome! I just had some security-related concerns and a couple of nits. Let me know what you think
|
|
||
| // strip event-handler attributes (already removed by the allowlist | ||
| // when !allowSafeAttributes; this guards the permissive path) | ||
| if (lowerName.startsWith('on')) { |
There was a problem hiding this comment.
The old rich-content path ran through sanitizeDOMString, whose allowlist kept only class/id/href/src/name/slot. sanitizeDOMTree's permissive mode flips that to an everything-except-denylist: it keeps any attribute that isn't on*-prefixed or javascript:-valued. So style (CSS injection, background:url() beaconing, UI spoofing), data: URIs in href/src, formaction/action/target, and xlink:href on SVG <a> now survive where they were always stripped before. The javascript: guard also leans on element[attributeName] property reflection to catch entity-obfuscated payloads, and that's undefined for namespaced attrs, so something like java	script: in xlink:href would only hit the literal-substring value check and slip through.
I know this path is opt-in (innerHTMLTemplatesEnabled defaults to false) and the content is the developer's own ion-select-option markup, not raw user input, so the real-world exposure is narrow. And I get that the goal is to keep size/color/shape. Mostly I want a conscious sign-off here: would keeping an allowlist even in permissive mode (or at minimum stripping style and normalizing the URL-bearing attrs) be safer than the denylist? Not blocking, but worth a deliberate call since it widens what reaches the innerHTML/VNode sinks.
| }} | ||
| ></Tag> | ||
| <Tag class={className} key={keyPrefix}> | ||
| {Array.from(content.childNodes).map((child, i) => cloneToVNode(child, keyPrefix, i))} |
There was a problem hiding this comment.
cloneToVNode filters nothing by design and the doc leans on callers to pre-sanitize. The ion-select path is fine since getOptionContent runs sanitizeDOMTree first. The gap is renderOptionLabel: it accepts label: string | HTMLElement and reads startContent/endContent, none of which are on the public AlertInput type. A vanilla-JS consumer doing alertController.create({ inputs: [{ label: someEl, startContent: ... }] }) reaches cloneToVNode with DOM that never passed through sanitizeDOMTree, and nothing enforces the contract.
Could we call sanitizeDOMTree defensively inside renderClonedContent? It'd be idempotent for the already-sanitized select path, so it just closes the JS-consumer hole without changing the select behavior. Up to you though.
Co-authored-by: Shane <[email protected]>
Co-authored-by: Shane <[email protected]>
3 participants
Issue number: resolves internal
What is the current behavior?
ion-select-optiondoesn't exposelabelPlacementorjustifyprops<ion-icon>placed inside anion-select-optionrenders empty in Vue/React (and other frameworks that bind custom-element props as DOM properties)select-option-start/select-option-endaren't size-capped consistently: oversized contentalert.{ios,md,ionic}.scss,action-sheet.ionic.scss,select-{modal,popover}.{ios,md,ionic}.scss) import the wrong shared partialsWhat is the new behavior?
labelPlacementandjustifyprops toion-select-optionand pass them through to alert, popover, and modal overlay paths@use/@importchains in the affected theme stylesheets so each theme file pulls in the right shared partials (mixins, theme globals, common base) and no longer relies on transitive imports that weren't guaranteedreflectPropertiesToAttributestocore/src/utils/sanitization/and call it fromgetOptionContentimmediately beforecloneNode. The helper mirrors a registered set of custom-element DOM properties (icon,name,src,ios,mdonion-icon) onto attributes so the cloned overlay copy renders correctly regardless of how the framework bound the prop.Does this introduce a breaking change?
Other information
Dev build:
8.8.9-dev.11779411201.1a483e09Preview: